Remove ops that are not in parameters from tokenize_partial #368
Conversation
|
I’m not looking at the code at the moment, so I may need to revise later. However, I’m slightly confused about this. Don’t “unnamed” operands typically refer to blockwise dependencies that may be Expr objects? Wouldn’t these dependencies be an important part of the token? |
|
I agree generally speaking, but we are only using this in combine_similar at the moment where we definitely want to ignore other expressions in most cases (all cases at the moment). I am open to introduce an option to configure this if we need it at a later point. |
Interesting. I'll try to think about and review this change now, and then I'll probably agree with you. My current question is "how can we combine two expressions that depend on different operands? That would be a bug", but a review of the code may clear this up for me.
Yeah, that makes sense. Would it be overkill to add an |
Sure, we wouldn't be using it which turned me away from it, but not generally opposed
Merge is modifying 2 at once to make this possible. I assume that we will have to go a similar route for concat. A multi-column assign is tricky as well, don't have a good solution yet |
There was a problem hiding this comment.
Okay, I just got a chance to look through the code and I think I understand why you are saying that we don't need to consider unnamed operands.
If I understand correctly, the change should probably be in _find_similar_operations instead of _tokenize_partial. It seems like _tokenize_partial is used in places like are_co_aligned, where it is possible that we really do care about unnamed operands. In _find_similar_operations, however, we really don't care about these operands, because the only place where were aren't effectively using _find_similar_operations as a proxy for find_operations is in BlockwiseIO (where there are not unnamed parameters anyway).
My suggestion is that we change the ignore: list | None argument to ignore_operands: list | bool in _find_similar_operations. E.g.:
de _find_similar_operations(self, root: Expr, ignore_operands: list | bool = True):
....If ignore_operands=True is specified, we don't need to call _tokenize_partial at all, because alike already includes all "similar" operations when its operands are ignored. I'm not sure that there is a case where ignore_operands=False makes sense, but we would just pass through ignore=None to _tokenize_partial.
Does this make sense?
cc @rjzamora
I'd rather include those than include them. We don't care about those at the moment, we can still add an option if we want them at some point, but they cause trouble with the multi-column assign